-
Notifications
You must be signed in to change notification settings - Fork 315
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Remove deprecated code warnings and minor perf improvements #1436
Conversation
5665596
to
a82b874
Compare
parents | ||
.into_par_iter() | ||
.map(|parent| t_aux.column(parent).expect("failed to get parent column")) | ||
.collect::<Vec<Column<Tree::Hasher>>>(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you should be able to collect into Result<Vec>>
allowing you to acutally handle the error, instead of expect
ing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe I'm missing something, but it looks like not what I want. The return type is Result<Vec<Column<Tree::Hasher>>>
, but w/o the expect
, we'd be returning Vec<Result<Column<Tree::Hasher>>>
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
parents
.into_par_iter()
.map(|parent| t_aux.column(parent))
.collect::<Result<Vec<Column<Tree::Hasher>>, _>>()?
is what I have in mind, to avoid the expect
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hrm, that makes sense, I think I had simply noted the difference of return type if the expect was removed, not changing the collect type. Is there a difference between:
.collect::<Result<Vec<Column<Tree::Hasher>>, _>>()?
and
.collect::<Result<Vec<Column<Tree::Hasher>>>()?
They both appear to work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They both appear to work.
Nevermind, just seems to be the type of error, which is implied either way (explicitly or implicitly).
a82db80
to
e6a4746
Compare
Rebased against master |
feat: add some minor parallelization where we can
e6a4746
to
6e479b9
Compare
Aims to resolve #1435 and investigate #1426